Fix some daemon crashes involving classes becoming generic#8157
Fix some daemon crashes involving classes becoming generic#8157
Conversation
Fixes #3279. Also fixes another related crash.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Great, fewer crashes!
An idea about testing: have you considered also testing this by turning some classes in the mypy implementation to generic (and back)?
|
|
||
| [out] | ||
| == | ||
| b.py:7: note: Revealed type is 'def () -> b.C[Any]' |
There was a problem hiding this comment.
What about a similar test case where a class goes from generic to non-generic?
| for i in range(len(instance.args)): | ||
| # N.B: We use zip instead of indexing because the lengths might have | ||
| # mismatches during daemon reprocessing. | ||
| for tvar, mapped_arg, instance_arg in zip(tvars, mapped.args, instance.args): |
There was a problem hiding this comment.
It's kind of unfortunate that we have to do this. Another option might be to make args a property and have it adjust the number of args dynamically if the TypeInfo has changed. This could have performance impliciations, however.
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
This is unfortunate. What about returning something obviously wrong (like we do in fixup.py) for the temporary results? Otherwise this may mask some other bugs.
Also why not fix a similar issue in mypy.join.join_instances()? What about a test where number or type variables changes from 2 to 1 and/or vice versa?
|
I didn't do join_instances because I couldn't manage to make it crash and because I had convinced myself from a glance that it couldn't break because of some other side condition, but I don't think that is actually true. I don't love the idea of adjusting things as an |
mypy/meet.py
Outdated
| if isinstance(self.s, Instance): | ||
| si = self.s | ||
| if t.type == si.type: | ||
| if t.type == si.type and len(t.args) == len(si.args): |
There was a problem hiding this comment.
Btw why do we need the and ... here if we anyway use zip() below?
There was a problem hiding this comment.
Oh, no, we definitely do not. The and was my first approach for fixing this, before I decided I liked the zip version more. (This check does work, though, so that could be an approach?)
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Just to clarify, this is good to go, but couple more tests that Jukka and me proposed would be better.
3 participants
Fixes #3279. Also fixes another related crash.